1221, 1222: Speed up get_pairwise_comparisons() and skip unused tests in add_relative_skill() - #1227
1221, 1222: Speed up get_pairwise_comparisons() and skip unused tests in add_relative_skill()#1227seabbs-bot wants to merge 4 commits into
Conversation
Pivot the scores of each group into a forecast unit by comparator matrix once, instead of merging the scores separately for every pair of comparators. The overlapping forecasts of a pair are the rows where both columns are non-missing. Results are identical to the previous implementation. The per-pair statistics are moved into a new internal helper, .compare_scores(), which is shared with compare_forecasts(). The latter is retained as a reference implementation and is used in tests to check the pivot-based implementation against the per-pair merge. Scores with more than one row per forecast unit and comparator now produce an informative error instead of silently comparing duplicated rows. Closes #1221 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpZEQ26Z76K1a3SxFKkR17
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1227 +/- ##
==========================================
+ Coverage 98.38% 98.41% +0.02%
==========================================
Files 41 41
Lines 2235 2274 +39
==========================================
+ Hits 2199 2238 +39
Misses 36 36 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
add_relative_skill() drops the p-value columns before returning, but previously still ran a Wilcoxon test for every pair of comparators. It now has an explicit test_type argument defaulting to NULL so that no test is run unless requested. Relative skill scores are unchanged and spurious wilcox.test() warnings for tied scores no longer appear. Closes #1222 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpZEQ26Z76K1a3SxFKkR17
seabbs-bot
changed the title
Use a small subset with tied but non-zero paired differences so that wilcox.test() warns about ties on all supported R versions. Identical scores for two models no longer trigger the warning on R 4.6. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpZEQ26Z76K1a3SxFKkR17
Mock wilcox.test() and count its calls instead of relying on its warnings, whose behaviour with ties differs across R versions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpZEQ26Z76K1a3SxFKkR17
![seabbs-review-bot[bot]](https://avatars.githubusercontent.com/in/4624046?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR pivots scores once into a forecast-unit-by-comparator matrix instead of re-merging per pair, and skips the wilcoxon test by default in add_relative_skill(). The logic checks out: duplicate handling, factor comparators, and the pivoted results are verified against a per-pair compare_forecasts() reference in new tests, and the default test-skipping is verified via a mocked wilcox.test() call count. Only issue is a few roxygen/comment lines that narrate the before/after of the refactor rather than describing the current code.
Automated first pass by seabbs-review-bot (Claude sonnet), triggered by: first pass. Not a human review. Comment @seabbs-review-bot to ask for another pass: @seabbs any time, the author's agent once it has pushed changes. Add the no-review label to opt this PR out. Ping @seabbs with any questions.
| #' are found by merging the scores of the two comparators on the forecast | ||
| #' unit. The actual comparison is then done by [.compare_scores()]. | ||
| #' | ||
| #' [pairwise_comparison_one_group()] no longer calls this function for every |
There was a problem hiding this comment.
suggestion This @description for compare_forecasts() narrates the refactor ("no longer calls this function for every pair... instead") rather than describing current behaviour. Rewrite to state the present fact, e.g. "pairwise_comparison_one_group() does not call this function; it aligns all comparators at once via .pivot_scores(). compare_forecasts() is kept as a reference implementation for testing."
| forecast_unit <- get_forecast_unit(scores) | ||
| merge_by <- setdiff(forecast_unit, compare) | ||
|
|
||
| # remove exact duplicates once here, instead of inside every pairwise merge |
There was a problem hiding this comment.
suggestion "remove exact duplicates once here, instead of inside every pairwise merge" narrates the old per-pair approach that no longer exists. A comment describing what this does now (e.g. "drop exact duplicate rows before checking for genuine forecast-unit duplicates") would read cleanly without referencing the prior implementation.
| #' forecasts it calls [compare_forecasts()]. | ||
| #' that subgroup is managed from [pairwise_comparison_one_group()]. | ||
| #' | ||
| #' Internally, the scores are pivoted once into a matrix with one row per |
There was a problem hiding this comment.
suggestion This paragraph explains the pivot approach by contrasting it with the old compare_forecasts()-per-pair approach ("avoids re-joining... considerably faster than calling compare_forecasts() for every pair"). Fine as a one-off note, but consider trimming to describe what the function does now, leaving the historical comparison to the PR description/NEWS.
2 participants
Description
This PR closes #1221 and closes #1222.
Pivot scores once (#1221)
pairwise_comparison_one_group()previously re-merged the scores of two comparators for every pair of comparators.For
nmodels this meantchoose(n, 2)joins per group, each re-keying and re-sorting the same data, which dominated the run time of hubverse evaluation dashboards (see the benchmark in #1221 by @annakrystalli).This PR follows the approach proposed in the issue:
.pivot_scores().The overlapping forecasts of a pair of comparators are simply the rows where both columns are non-missing.
.compare_scores().compare_forecasts()is retained as a simple per-pair reference implementation and now delegates to.compare_scores().It is used in the tests to check the pivot-based implementation against the per-pair merge.
Exact duplicate rows are still removed, as before.
Results are identical to the previous implementation.
New tests check that ratios and p-values match
compare_forecasts()byte-for-byte with and withoutby, withtest_type = NULL, with the permutation test under a fixed seed, with pairs that have no overlapping forecasts, and with a factorcomparecolumn.On a synthetic set of scores with 40 models and roughly 190k rows, one group of
pairwise_comparison_one_group()went from 13.7 s and 9.2 GB allocated to 5.2 s and 1.3 GB allocated, with identical output.Skip the discarded test in
add_relative_skill()(#1222)add_relative_skill()drops thepvalandadj_pvalcolumns before returning but previously still ran a Wilcoxon test for every pair of comparators.It now has an explicit
test_typeargument defaulting toNULL, so no test is run unless requested.Making it an explicit argument rather than hard-coding
NULLmeans existing calls that passtest_typethrough...keep working.Relative skill scores are unchanged, and spurious
wilcox.test()warnings for tied scores no longer appear.A test mocks
wilcox.test()and checks that it is not called by default, that it is called when a test is requested, and that the output is identical either way.Checklist
lintr::lint_package()to check for style issues introduced by my changes.🤖 Generated with Claude Code
https://claude.ai/code/session_01LpZEQ26Z76K1a3SxFKkR17